Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ticks.sampleSize option #6508

Merged
merged 1 commit into from
Oct 9, 2019
Merged

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Sep 7, 2019

This allows users to set a sampleSize option which is a major performance improvement when there's a large number of ticks / labels

The charts render nearly twice as fast with this option. Here's timing information for samples/scales/time/financial.html changed to render 8,000 datapoints:
Without this PR: 520ms
With sampleSize: 100: 272ms

Plunker: https://plnkr.co/edit/Bj6unMv5JSL69nfUZ3zx?p=preview

@benmccann benmccann force-pushed the autoskip-sampling branch 4 times, most recently from a635144 to fa09223 Compare September 8, 2019 02:13
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea, but I don't like the implementation. Its getting really hard to follow what is contained in ticks or me.ticks or me._ticks at different stages and why.

Could the same performance gain be achieved by just changing computeLabelSizes to use sampleSize?

src/core/core.scale.js Outdated Show resolved Hide resolved
src/core/core.scale.js Outdated Show resolved Hide resolved
src/core/core.scale.js Outdated Show resolved Hide resolved
@benmccann benmccann force-pushed the autoskip-sampling branch 2 times, most recently from 43ef566 to 7b66d0c Compare September 11, 2019 17:25
@benmccann
Copy link
Contributor Author

benmccann commented Sep 11, 2019

Could the same performance gain be achieved by just changing computeLabelSizes to use sampleSize?

Unfortunately, it's a little more complicated than that. The main thing we want to do to achieve the performance gains is have convertTicksToLabels happen after auto-skipping. Unfortunately, it's almost impossible to re-order the methods because there are so many inter-dependencies between. them. E.g. _autoSkip relies on fit and calculateTickRotation having already been called. calculateTickRotation and _autoSkip both rely on _getLabelSizes which relies on _ticks having been defined, etc. Rather than reordering too much, I found it much simpler to call _convertTicksToLabels twice.

I like the idea, but I don't like the implementation. Its getting really hard to follow what is contained in ticks or me.ticks or me._ticks at different stages and why.

I agree that it's quite hard to follow ticks vs me.ticks vs me._ticks. Though I think that problem exists quite a bit in master as well and am not sure this PR makes it much worse. I reverted a change I made to afterBuildTicks. Perhaps it's clearer now? I really can't wait to be able to get rid of me.ticks in v3 because it makes this code extremely difficult to work with.

src/core/core.scale.js Outdated Show resolved Hide resolved
docs/general/performance.md Show resolved Hide resolved
src/core/core.scale.js Show resolved Hide resolved
@benmccann benmccann requested a review from kurkle October 7, 2019 04:41
src/core/core.scale.js Show resolved Hide resolved
src/core/core.scale.js Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants